Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

raise_on_encrypted: true #177

Merged
merged 5 commits into from
Apr 4, 2023
Merged

raise_on_encrypted: true #177

merged 5 commits into from
Apr 4, 2023

Conversation

leviwilson
Copy link
Contributor

This pull request adds a raise_on_encrypted: true option to CombinePDF.load. In this manner, a consumer can choose to have CombinePDF throw an exception in the event that the PDF they are trying to open is encrypted.

In addition to this, sometimes during the CombinePDF::PDFParser#parse, it can come across a scenario where it raises a Zlib::DataError but was still able to obtain some information about the file in the root_object such as whether or not there is root_object[:Encrypt] information in it. For those conditions, rather than raising a Zlib::DataError it will raise the CombinePDF::EncryptionError instead but only if the raise_on_encrypted: true has been set. Otherwise, it'll allow the Zlib::DataError to bubble up.

@leviwilson leviwilson mentioned this pull request Jul 30, 2020
Closed
@leviwilson
Copy link
Contributor Author

@boazsegev any feedback on these changes?

@boazsegev
Copy link
Owner

Sorry, busy days... I'll look it over soon... I hope.

@takahashim
Copy link

This PR is great! 👏
The :raise_on_encrypted option and CombinePDF::EncryptionError are exactly what I was looking for. I'm very happy to see these two features merged.

On the other hand, there are some things I've been wondering about in the PR.

  • test/combine_pdf/load_test.rb is spec style, but test/combine_pdf/renderer_test.rb is Minitest::Test style. I don't think it's very desirable to have two specs with different styles in there.
  • Fixing the Rakefile for test runs is great, but it solves a different problem than encryption and EncryptionError. I think it would be better to separate it into another Pull Request.
    • Ditto for adding the .travis.yml.

@leviwilson
Copy link
Contributor Author

  • test/combine_pdf/load_test.rb is spec style, but test/combine_pdf/renderer_test.rb is Minitest::Test style. I don't think it's very desirable to have two specs with different styles in there.

I've never used Minitest before but the load_test.rb had more verifications which left me seeking additional Minitest functionality, but I still stuck within Minitest

  • Fixing the Rakefile for test runs is great, but it solves a different problem than encryption and EncryptionError. I think it would be better to separate it into another Pull Request.

It's still related to the PR since when I went to run specs, there was no way to run all of them. Thought it would be best to make it a default rake task like other projects.

  • Ditto for adding the .travis.yml.
    I'm indifferent. Seems innocuous enough to not warrant another PR.

Thanks for the feedback / looking at it!

@leviwilson
Copy link
Contributor Author

@boazsegev just checking in on this. It look good?

@reiz
Copy link

reiz commented Oct 12, 2021

@boazsegev @leviwilson that would be a great feature.

@kimyu92
Copy link

kimyu92 commented Apr 1, 2023 •
edited
Loading

@boazsegev Do you mind to take a look on this feature to at least allow user to stop proceeding until a better decryption is implemented?

There is no way user can stop proceeding encrypted pdf without able to ingest flag

boazsegev
boazsegev requested changes Apr 1, 2023
View reviewed changes
Copy link
Owner

@boazsegev boazsegev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work. I apologize for the (extremely) belated response.

I would rather avoid the catch-and-release for the Zlib::DataError exception unless it is always true that the exception is caused by an encryption error (and I think it could be caused by a PDF object compression error as well)...

Is it possible to remove this before a merge?

Thanks!

lib/combine_pdf/pdf_public.rb Show resolved Hide resolved
@boazsegev
Copy link
Owner

Thank you every one for the work you put in on this :)

I'll test and push the patch as soon as I have a moment.

@boazsegev boazsegev merged commit 34118fb into boazsegev:master Apr 4, 2023
@boazsegev
Copy link
Owner

released @ 1.0.23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kimyu92 kimyu92 kimyu92 left review comments

@boazsegev boazsegev boazsegev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@leviwilson @boazsegev @takahashim @reiz @kimyu92